Skip to content

Scoping questions#1314

Closed
ibuildthings-instrumentl wants to merge 20 commits intocoleam00:devfrom
instrumentl:scoping-questions
Closed

Scoping questions#1314
ibuildthings-instrumentl wants to merge 20 commits intocoleam00:devfrom
instrumentl:scoping-questions

Conversation

@ibuildthings-instrumentl
Copy link
Copy Markdown

@ibuildthings-instrumentl ibuildthings-instrumentl commented Apr 20, 2026

Summary

Describe this PR in 2-5 bullets:

  • Problem:
  • Why it matters:
  • What changed:
  • What did not change (scope boundary):

UX Journey

Before

(Draw the user-facing flow BEFORE this PR. Show each step the user takes.)

Example:
  User                   Archon                   AI Client
  ────                   ──────                   ─────────
  sends message ──────▶  resolves session
                         loads context
                         streams to AI ──────────▶ processes prompt
                         receives chunks ◀──────── streams response
  sees reply ◀─────────  sends to platform

After

(Draw the user-facing flow AFTER this PR. Highlight what changed with [brackets] or asterisks.)

Architecture Diagram

Before

(Map ALL modules touched or connected to this change. Draw lines between them.)

After

(Same diagram with changes highlighted. Mark new modules with [+], removed with [-],
 modified with [~]. Mark new connections with ===, removed with --x--.)

Connection inventory (list every module-to-module edge, mark changes):

From To Status Notes
unchanged / new / removed / modified

Label Snapshot

  • Risk: risk: low|medium|high
  • Size: size: XS|S|M|L|XL
  • Scope: core|workflows|isolation|git|adapters|server|web|cli|paths|config|docs|dependencies|ci|tests|skills
  • Module: <scope>:<component> (e.g. workflows:executor, adapters:slack, core:orchestrator)

Change Metadata

  • Change type: bug|feature|refactor|docs|security|chore
  • Primary scope: core|workflows|isolation|git|adapters|server|web|cli|paths|multi

Linked Issue

  • Closes #
  • Related #
  • Depends on # (if stacked)
  • Supersedes # (if replacing older PR)

Validation Evidence (required)

Commands and result summary:

bun run type-check
bun run lint
bun run format:check
bun run test
# Or all at once:
bun run validate
  • Evidence provided (test/log/trace/screenshot):
  • If any command is intentionally skipped, explain why:

Security Impact (required)

  • New permissions/capabilities? (Yes/No)
  • New external network calls? (Yes/No)
  • Secrets/tokens handling changed? (Yes/No)
  • File system access scope changed? (Yes/No)
  • If any Yes, describe risk and mitigation:

Compatibility / Migration

  • Backward compatible? (Yes/No)
  • Config/env changes? (Yes/No)
  • Database migration needed? (Yes/No)
  • If yes, exact upgrade steps:

Human Verification (required)

What was personally validated beyond CI:

  • Verified scenarios:
  • Edge cases checked:
  • What was not verified:

Side Effects / Blast Radius (required)

  • Affected subsystems/workflows:
  • Potential unintended effects:
  • Guardrails/monitoring for early detection:

Rollback Plan (required)

  • Fast rollback command/path:
  • Feature flags or config toggles (if any):
  • Observable failure symptoms:

Risks and Mitigations

List real risks in this PR (or write None).

  • Risk:
    • Mitigation:

Summary by CodeRabbit

New Features

  • Scout APM Performance Optimization Workflow: Automated discovery of slow HTTP routes and consolidated optimization planning
  • Slack Feature-to-Review-App Workflow: End-to-end process converting feature requests into ready-for-review PRs with deployed review apps
  • Interactive Workflow Gates: Approve/Request Changes buttons in Slack for streamlined approval workflows
  • Message Receipt Acknowledgment: Slack bot now posts eyes reaction on incoming messages

Documentation

  • Updated Slack bot setup guide with new reactions:write permission requirement

ibuildthings-instrumentl and others added 20 commits April 17, 2026 11:39
- Added a new configuration option to copy local .env files into isolated worktrees for Scout MCP compatibility.
- Removed outdated note regarding Claude Code binary path from setup documentation.
- Renumbered subsequent steps in the setup guide for clarity.
Update Archon configuration and setup documentation
Add design for a Slack @archie bot flow that takes a natural-language feature
request end-to-end: interactive spec with bounded revision loop, plan, implement
in worktree, open PR, bounded review loop, wait for CI, trigger review-app
deploy, post URL back to the thread. Composes existing Archon commands plus
three small helper scripts; no adapter changes required.

Made-with: Cursor
Bite-sized tasks covering 3 helper scripts, the workflow YAML, bundled-
defaults registration, pre-PR validation, and a manual smoke-test
checklist. Noted divergences from the design doc: code-review rounds
are unrolled explicitly, reviewApp config is hardcoded for v1, and
per-script unit tests are dropped in favor of workflow-level parsing
plus manual smoke test.

Made-with: Cursor
Wraps gh workflow run for review-app deployment; exits non-zero with a
clear message on dispatch failure. Used by archon-slack-feature-to-review-app.

Note: written as .js (not .ts as originally planned) to match the existing
.archon/scripts/echo-args.js pattern and avoid the typed-linting scope gap
for .archon/scripts/**/*.ts.

Made-with: Cursor
Wraps gh pr checks --watch --fail-fast with a wall-clock timeout so the
workflow can't hang indefinitely. Exit codes distinguish pass/fail/timeout.

Note: written as .js (not .ts as originally planned) for the same reason
as dispatch-review-app.js.

Made-with: Cursor
Polls gh pr view --json comments for a URL matching a caller-supplied
regex; prints the URL on stdout, errors on stderr so the workflow engine
captures only the URL via \$nodeId.output.

Note: written as .js (not .ts as originally planned) for the same reason
as dispatch-review-app.js.

Made-with: Cursor
End-to-end workflow for Slack @archie feature requests: interactive spec
creation (bounded 3-iteration revision loop), plan + implement + PR using
existing commands, two-round code review with conditional second pass, CI
wait, review-app dispatch, URL fetch from PR comments, and final post back
to the Slack thread. Composes existing commands; adds no new adapter or
orchestrator code.

Script invocations use .js extensions per the Tasks 1-3 divergence.

Made-with: Cursor
Adds the text import + map entry so binary builds include the workflow.
Bumps the bundled-workflow count assertion from 13 to 14 and adds the
workflow to the expected-names list.

Made-with: Cursor
Insert an interactive refine-plan loop between create-plan and
plan-setup, mirroring the existing spec-approval gate and the pattern
used by archon-scout-perf-roadmap. The loop posts a condensed plan
summary in-thread, accepts feedback that edits $ARTIFACTS_DIR/plan.md
in place, and only proceeds to plan-setup on explicit "approved" /
"looks good" / "ship it" / "go". Bounded at max_iterations: 5.

Rationale: previously the workflow jumped straight from plan creation
into implementation, giving the user no chance to reshape scope,
ordering, or task list before code gets written. This symmetrizes the
gating with Phase A and matches how other plan-driven workflows behave.

Made-with: Cursor
Interactive-loop gate messages now render Approve (primary) and Request
changes buttons in Slack; clicking Approve resumes the paused workflow,
while Request changes opens a modal with a feedback textarea whose
submission is synthesized into the gate thread.

- packages/core: add optional `interactiveGate` to MessageMetadata.
- packages/workflows: dag-executor gate-send passes runId + nodeId via
  the new metadata field so adapters can bind actions per run.
- packages/adapters/slack: sendMessage renders an actions block on the
  final chunk when the gate metadata is present; Bolt action + view
  handlers synthesize message events that reuse the existing
  natural-language approval path in handleMessage, so no new server
  wiring is required.
- Fallback path: adapters without rich input ignore the metadata; the
  text body still includes the `/workflow approve <uuid>` instructions.

Tests:
- 3 new Slack adapter tests asserting the actions block, action_ids,
  and that buttons attach only to the final chunk of long messages.
- 1 extra assertion on the dag-executor interactive-loop test verifying
  the gate-send carries { runId, nodeId } metadata.

Made-with: Cursor
feat: @archie Slack feature-to-review-app workflow
feat(slack): Block Kit approve + request-changes UI for workflow loop gates
When a user @mentions or DMs Archon, the bot now posts an 👀 reaction
on the incoming message the moment it's received -- before thread-history
fetch, lock acquisition, planner warm-up, or first LLM token. This
eliminates the awkward silent gap between "user hit send" and "bot
responds" for long-running workflows.

- SlackAdapter.acknowledgeReceipt(event) calls reactions.add; swallows
  errors so a missing reactions:write scope just skips the reaction
  instead of blocking the conversation.
- Server onMessage handler fires the ack right after auth/stripBotMention
  with void (fire-and-forget) so the reaction round-trip never delays
  orchestration.
- reactions:write added to the Starlight docs, skill guide, and CLI
  setup prompt as an optional scope.
- Three adapter tests cover the happy path, missing-scope failure, and
  already_reacted replay case.

Made-with: Cursor
feat(slack): ack incoming messages with 👀 reaction
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
fix(deps): bump flatted to >=3.4.2 (Dependabot #19)
Fixes SSRF via NO_PROXY Hostname Normalization Bypass and Cloud Metadata Exfiltration via Header Injection Chain. Transitive from @slack/bolt and @slack/web-api.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nput

Add a new implementation plan for converting scoping questions into a Slack modal form. This includes modifications to the workflow prompt contract, Slack adapter for parsing and rendering the question schema, and handling modal submissions. The changes ensure that the workflow remains intact while enhancing user interaction through structured inputs. Related files updated include the workflow YAML, Slack adapter, and corresponding tests.
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 20, 2026

📝 Walkthrough

Walkthrough

A comprehensive feature PR introducing Scout APM performance optimization automation and Slack-driven feature-to-review-app workflows. Adds new Archon commands and workflows, Slack adapter interactive gate support, helper scripts for CI/deployment, type system extensions, and configuration/setup updates to enable end-to-end APM profiling and code-review automation pipelines.

Changes

Cohort / File(s) Summary
Scout APM Integration
.archon/commands/defaults/scout-discover-routes.md, scout-consolidate-perf-plan.md, .archon/workflows/defaults/archon-scout-perf-roadmap.yaml
New Scout MCP-based commands to discover high-impact HTTP routes, profile them in parallel, and consolidate findings into a performance optimization plan.
Slack-driven Feature Workflow
.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml, docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md, docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md
New interactive workflow orchestrating Slack feature requests through spec creation, implementation, PR review, CI validation, review-app deployment, and progress posting back to Slack.
Helper Scripts for CI & Deployment
.archon/scripts/ci-wait.js, dispatch-review-app.js, fetch-review-app-url.js
Three new utility scripts: waiting on GitHub CI with timeout enforcement, dispatching review-app workflows, and polling PR comments for deployment URLs.
Slack Adapter & Interactive Gate Support
packages/adapters/src/chat/slack/adapter.ts, adapter.test.ts
Enhanced Slack adapter with Approve/Request-changes buttons in gate messages, :eyes: receipt acknowledgment, and action encoding/decoding for gate tracking.
Type System Extensions
packages/core/src/types/index.ts, packages/workflows/src/deps.ts, packages/workflows/src/dag-executor.ts
Added interactiveGate metadata field to enable platform adapters to identify and render rich gate controls; updated gate message delivery to include runId and nodeId.
Workflow Registration
packages/workflows/src/defaults/bundled-defaults.ts, bundled-defaults.test.ts
Registered new archon-slack-feature-to-review-app workflow in bundled defaults; updated test expectations.
Configuration & Setup
.archon/config.yaml, packages/cli/src/commands/setup.ts, .claude/skills/archon/guides/setup.md, slack.md
Added .env file copying to worktree setup; registered reactions:write Slack scope in setup wizard and documentation; cleaned up binary path note in setup guide.
Platform & Server Updates
packages/server/src/index.ts, packages/workflows/src/dag-executor.test.ts
Integrated receipt acknowledgment call in Slack message handler; extended gate-message test to verify interactiveGate metadata inclusion.
MCP & State Files
.cursor/mcp.json, .cursor/hooks/state/continual-learning.json, continual-learning-index.json
Added Cursor MCP configuration for Scout APM Docker container; initialized continual-learning hook state tracking.
Documentation & Dependencies
AGENTS.md, package.json, docs/plans/2026-04-20-slack-scoping-questions-form-plan.md, packages/docs-web/src/content/docs/adapters/slack.md
Added learned workspace facts and preferences; pinned flatted and axios dependencies; documented future scoping-questions modal enhancement; updated Slack adapter documentation.

Sequence Diagram

sequenceDiagram
    actor User as Slack User
    participant Slack as Slack Thread
    participant Archon as Archon Platform
    participant GH as GitHub
    participant Deploy as Review App<br/>Deployment
    
    User->>Slack: Feature request message
    Slack->>Archon: Message with feature request
    
    Note over Archon: Spec Creation Loop<br/>(max 3 iterations)
    Archon->>Slack: Request spec approval
    Slack->>User: Spec prompt with Approve/Request buttons
    User->>Slack: Approve specification
    Slack->>Archon: SPEC_APPROVED signal
    
    Note over Archon: Plan Generation & Review
    Archon->>Archon: Generate implementation plan
    Archon->>Slack: Request plan approval
    User->>Slack: Approve plan
    Slack->>Archon: PLAN_APPROVED signal
    
    Archon->>Archon: Implement tasks on isolated worktree
    Archon->>GH: Create draft PR
    
    Note over Archon: Two-Round Code Review
    Archon->>Archon: Round 1: Run parallel review agents
    alt Findings exist
        Archon->>Archon: Apply automated fixes
        Archon->>Archon: Round 2: Re-run review agents
    end
    
    Archon->>GH: Wait for CI completion (with timeout)
    GH->>Archon: CI passed
    
    Archon->>GH: Dispatch review-app workflow
    Deploy->>GH: Post review-app URL in PR comments
    Archon->>GH: Poll PR comments for URL
    GH->>Archon: Extract review-app URL from regex match
    
    Archon->>Slack: Post final completion with PR + review-app URLs
    Slack->>User: Done! Feature ready for testing
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 A bunny's workflow hops through Slack,
From feature dreams to code feedback.
Scout finds bottlenecks in flight,
While Archon reviews, refines, and lights
The path from ask to review-app delight! 🚀

🚥 Pre-merge checks | ❌ 3

❌ Failed checks (2 warnings, 1 inconclusive)

Check name Status Explanation Resolution
Description check ⚠️ Warning The PR description is an empty template with all required sections (Summary, UX Journey, Architecture Diagram, validation evidence, security impact, compatibility, rollback plan, risks) left blank—no concrete details, problem statement, validation results, or mitigation strategies are provided. Complete the template with: problem/impact bullets, before/after UX flows, architecture diagrams, validation command outputs, security assessment (yes: new permissions reactions:write and external gh calls), compatibility details, and rollback instructions.
Docstring Coverage ⚠️ Warning Docstring coverage is 18.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title "Scoping questions" is vague and does not clearly summarize the main change in the changeset, which involves a large, multi-part feature spanning new workflows, Slack adapter enhancements, helper scripts, and configuration updates. Replace with a more descriptive title that captures the primary feature, such as "Add Slack-driven feature-to-review-app workflow with scoping questions modal" or "Implement interactive Slack workflow for feature requests with plan approval gates".

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch scoping-questions

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 16

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
.claude/skills/archon/guides/setup.md (1)

161-173: ⚠️ Potential issue | 🟡 Minor

Fix subsection numbering: these should remain under Step 4, not Step 5.

After 4b, the next subsections should be 4c and 4d. Using 5c/5d conflicts with the actual ## Step 5 that starts at Line 190 and makes the flow ambiguous.

Suggested doc fix
-### 5c: Verify Configuration
+### 4c: Verify Configuration
...
-### 5d: Run Database Migrations (PostgreSQL only)
+### 4d: Run Database Migrations (PostgreSQL only)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.claude/skills/archon/guides/setup.md around lines 161 - 173, The two
subsection headings currently labeled "### 5c: Verify Configuration" and "###
5d: Run Database Migrations (PostgreSQL only)" are incorrectly numbered and
should remain under Step 4; rename those headings to "### 4c: Verify
Configuration" and "### 4d: Run Database Migrations (PostgreSQL only)"
respectively so they follow "4b" and avoid conflict with the actual "## Step 5"
section; ensure any internal references to 5c/5d in nearby text are updated to
4c/4d as well.
packages/workflows/src/defaults/bundled-defaults.ts (1)

40-40: ⚠️ Potential issue | 🟡 Minor

Stale count in section comment.

Header still says "13 total" but BUNDLED_WORKFLOWS now registers 14 entries. Minor doc drift but worth keeping in sync with the test's length assertion.

✏️ Proposed fix
-// Default Workflows (13 total)
+// Default Workflows (14 total)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/workflows/src/defaults/bundled-defaults.ts` at line 40, The section
header comment "// Default Workflows (13 total)" is out of sync with
BUNDLED_WORKFLOWS (which now contains 14 entries); update the header to reflect
the correct count (e.g., "// Default Workflows (14 total)"), or replace the
hardcoded count with a dynamic reference in documentation/tests by using
BUNDLED_WORKFLOWS.length where appropriate so the comment/test stays in sync
with the array.
🧹 Nitpick comments (3)
packages/workflows/src/deps.ts (1)

50-51: Extract the public interactiveGate shape into an interface.

This is part of an exported metadata contract, so naming the shape makes future adapter/core alignment clearer. As per coding guidelines, use interface for defining object shapes in TypeScript; prefer interfaces over type aliases for public API contracts.

♻️ Proposed refactor
+export interface WorkflowInteractiveGateMetadata {
+  runId: string;
+  nodeId: string;
+}
+
 export interface WorkflowMessageMetadata {
@@
   workflowDispatch?: { workerConversationId: string; workflowName: string };
   workflowResult?: { workflowName: string; runId: string };
   /** Mirror of MessageMetadata.interactiveGate — see packages/core/src/types/index.ts. */
-  interactiveGate?: { runId: string; nodeId: string };
+  interactiveGate?: WorkflowInteractiveGateMetadata;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/workflows/src/deps.ts` around lines 50 - 51, Extract the inline
interactiveGate object shape into a named exported interface (e.g.,
InteractiveGate) and replace the optional property declaration interactiveGate?:
{ runId: string; nodeId: string } with interactiveGate?: InteractiveGate; ensure
the new interface is exported from the same module so it becomes part of the
public metadata contract and matches MessageMetadata.interactiveGate semantics.
.cursor/mcp.json (1)

5-5: Pin the Scout MCP image instead of using latest.

Using latest makes local MCP behavior non-reproducible and can change without a PR. Prefer a specific version tag (e.g., scoutapp/scout-mcp-local:v1.2.3) or a digest for stable review/workflow runs.

"args": ["run", "--rm", "-i", "--env", "SCOUT_API_KEY", "scoutapp/scout-mcp-local:latest"],
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.cursor/mcp.json at line 5, The args entry is using an unpinned image tag
"scoutapp/scout-mcp-local:latest" which makes runs non-reproducible; update the
args array to reference a specific version tag or immutable digest (for example
replace "scoutapp/scout-mcp-local:latest" with "scoutapp/scout-mcp-local:vX.Y.Z"
or "scoutapp/scout-mcp-local@sha256:...") so the invocation in the "args" array
is pinned and stable across runs.
.archon/scripts/dispatch-review-app.js (1)

34-38: Route gh stderr to process.stderr, not stdout.

Both stdout and stderr from gh are currently printed to the script's stdout, followed by the JSON summary line. Since workflow bash/script nodes capture stdout as $deploy-review-app.output, any gh informational output (e.g. "✓ Created workflow_dispatch event") ends up mixed with the JSON line, so a downstream consumer can't cleanly JSON.parse($deploy-review-app.output) or rely on the output being a single JSON object. Sending diagnostic gh chatter to stderr keeps the node's stdout deterministic.

♻️ Proposed fix
-    if (stdout.trim()) console.log(stdout.trim());
-    if (stderr.trim()) console.log(stderr.trim());
+    if (stdout.trim()) process.stderr.write(stdout.trim() + '\n');
+    if (stderr.trim()) process.stderr.write(stderr.trim() + '\n');
     console.log(
       JSON.stringify({ dispatched: true, workflow: workflowFile, ref })
     );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/scripts/dispatch-review-app.js around lines 34 - 38, The script
currently prints both gh stdout and stderr to stdout which corrupts the JSON
summary; change the stderr handling so that when stderr.trim() is truthy it is
written to stderr (e.g., use process.stderr.write(...) or console.error(...))
instead of console.log; keep the stdout handling and the JSON.stringify line
unchanged so the final JSON summary remains the only thing emitted on stdout by
dispatch-review-app.js (refer to the stdout/stderr variables and the
JSON.stringify({ dispatched: true, workflow: workflowFile, ref }) line).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.archon/config.yaml:
- Around line 3-5: The config currently copies the full local .env (copyFiles: -
.env) into each worktree which duplicates secrets; instead create a
least-privilege file (e.g., scout.env) that contains only the Scout-specific
vars (SCOUT_API_KEY) and replace the .env reference in copyFiles with that
filename, or switch to managed env injection instead of file-copy; also update
the Cursor MCP's envFile setting to point to the same least-privilege scout.env
if file-based injection is still used so both MCPs read the minimal Scout-only
vars rather than the full .env.

In @.archon/scripts/ci-wait.js:
- Around line 43-54: The timeout handler sets timedOut and schedules a 2s grace
timer with .unref(), but because unrefed timers don't keep the process alive the
grace timer may never run and the child 'exit' handler returns early, dropping
the intended exit code; update the timeout logic in the timeout callback to stop
using .unref() on the grace setTimeout and in the child.on('exit', ...) handler
(referencing timedOut, timer, and child) call process.exit(3) directly when
timedOut is true before returning so the process reliably exits with code 3
after a timeout.

In @.archon/scripts/fetch-review-app-url.js:
- Around line 86-95: The current polling loop around pollOnce swallows all
errors and retries, which delays exiting for permanent failures; update the
catch block in the while loop that wraps pollOnce (in
.archon/scripts/fetch-review-app-url.js) to detect non-retryable errors and exit
immediately with code 2: specifically check for err.code === 'ENOENT' (missing
gh binary) and JSON parse failures (err instanceof SyntaxError or err.name ===
'SyntaxError' coming from JSON.parse) and call process.exit(2) with a clear
error log; for other transient errors keep the existing console.error and
continue retrying.

In @.archon/workflows/defaults/archon-scout-perf-roadmap.yaml:
- Around line 27-32: Add a new bash validation node (id:
ensure-routes-discovered) immediately after the discover-routes node that checks
the produced routes.json is not an empty array (e.g., fail if file missing or
content == "[]"), and make all profile nodes (profile-00, profile-01, etc.)
depend on ensure-routes-discovered instead of discover-routes so the DAG fails
early when discovery yields no routes; keep consolidate-plan dependency
unchanged so it can still run when appropriate.

In @.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml:
- Around line 373-381: The bash step passes a POSIX-style character class to
fetch-review-app-url.js which is later used in new RegExp(...) (see new RegExp
usage around line 62 in fetch-review-app-url.js); replace the POSIX `[:space:]`
usage with JavaScript whitespace `\s` in the pattern argument and ensure the
backslash is escaped correctly for the YAML/bash context (so the JS gets `\s` at
runtime and dots are escaped for the domain). Update the `fetch-review-url`
step's pattern string accordingly so the script receives a valid JS regex that
stops at whitespace.

In @.claude/skills/archon/guides/slack.md:
- Line 13: The Bot Token Scopes list currently implies all listed scopes are
required; mark the reactions:write scope as optional (e.g., “reactions:write
(optional — only used for the :eyes: receipt reaction)”) or move it into a
separate “Optional scopes” sentence to keep optional Slack scope wording
consistent; update the “Bot Token Scopes” line that lists app_mentions:read,
chat:write, reactions:write, channels:history, channels:join, im:history,
im:write, im:read to reflect this change.

In @.cursor/hooks/state/continual-learning-index.json:
- Around line 1-11: Remove the committed runtime hook state file
.cursor/hooks/state/continual-learning-index.json from the PR and stop tracking
that directory: run git rm --cached for
.cursor/hooks/state/continual-learning-index.json (and any other files under
.cursor/hooks/state/ like the listed JSONL entries
"280d685f-a48a-46c6-833f-323dd405d054/280d685f-a48a-46c6-833f-323dd405d054.jsonl",
"d2ae91bc-63ac-4028-88d4-97ce68f614a8/...",
"caf60f72-6726-464d-a2cf-d3e482e5d1a9/..."), add .cursor/hooks/state/ to
.gitignore (or at minimum ignore these continual-learning state files), and
amend the commit or create a new commit removing them so runtime metadata is not
tracked.

In @.cursor/hooks/state/continual-learning.json:
- Around line 1-8: This file (.cursor/hooks/state/continual-learning.json)
contains run-specific continual-learning snapshot fields (lastRunAtMs,
lastTranscriptMtimeMs, lastProcessedGenerationId, trialStartedAtMs) and should
not be versioned; remove it from the repo history/PR changes by untracking it
(git rm --cached) and add a pattern to ignore these hook-state artifacts (e.g.,
ignore .cursor/hooks/state/ or continual-learning.json) so future runtime
snapshots are not committed, and revert any accidental modifications in this PR
so only core PR changes remain.

In `@AGENTS.md`:
- Line 6: Update the ambiguous sentence in AGENTS.md so "Web" is explicit and
the two behaviors are separated: change the text around "Settings → Platform
Connections" to say that the Settings → Platform Connections page currently
hardcodes Slack, Telegram, Discord, and GitHub as "not connected", while the
"Web" tab (or "Web adapter status" / "Web UI runtime status") is the one that
reflects live adapter connection state; explicitly name "Web UI" or "Web tab" as
the live-status source so it's clear a working Slack bot may still appear
unconfigured on the Settings page until the UI is wired to the real platform
status.

In `@docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md`:
- Around line 7-8: The plan currently claims "No adapter or orchestrator
changes" but the PR actually introduces interactive-gate plumbing in workflow
execution (see packages/workflows/src/dag-executor.ts where interactiveGate
metadata is now passed), so update the plan text to accurately reflect that
workflow execution/adapter behavior is changing: mention the interactiveGate
metadata addition in dag-executor, note that workflow execution now propagates
interactive gating (affecting adapter/executor blast radius), and adjust the
other occurrences (around the referenced blocks at the top and the other
sections called out) so reviewers aren’t misled about scope.
- Around line 18-21: The plan incorrectly references helper script filenames
with a .ts extension (e.g., `.archon/scripts/dispatch-review-app.ts`,
`.archon/scripts/ci-wait.ts`, `.archon/scripts/fetch-review-app-url.ts` and the
workflow file
`.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml` calls)
while the implemented scripts are `.js`; update all occurrences of these script
names in the plan to use the actual `.js` filenames (replace each `.ts`
reference with the corresponding `.js` one) so copy-paste commands, workflow
invocations, and any listed helper script names (dispatch-review-app, ci-wait,
fetch-review-app-url) match the real files throughout the document (also apply
the same replacements at the other line ranges mentioned).

In `@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md`:
- Line 181: The fenced code block at the mentioned location is missing a
language tag causing markdown lint errors; update the triple-backtick fence to
include the language identifier "text" (i.e., use ```text) for the plain
progress-message example so the block is explicitly marked as plain text.
- Around line 44-45: References to the workflow location and helper script
extensions are stale: update any occurrences of the old workflow path string
"packages/workflows/src/defaults/workflows/" to the new location
".archon/workflows/defaults/" and change helper script filenames/extensions
referenced as ".ts" to ".js" (e.g., helper script imports or examples), then
scan and fix the other occurrences called out (the additional instances noted in
the comment) so all path and extension references are consistent.

In `@packages/adapters/src/chat/slack/adapter.ts`:
- Around line 492-494: The logs currently pass raw ctx.userId into getLog().info
for the 'slack.gate_approve_clicked' event (and the similar getLog().info call
around lines 550-552); replace the raw userId with the existing Slack user-id
masking utility used by the unauthorized-message logs (i.e., call the same
masking function used elsewhere—e.g., maskUserId(ctx.userId) or the project’s
PII/masking helper) so the getLog().info payload contains the masked user id
instead of ctx.userId for both 'slack.gate_approve_clicked' and the similar gate
log.
- Around line 668-688: In dispatchSyntheticMessage, enforce the Slack whitelist
before invoking messageHandler: check the adapter's allowedUserIds (e.g.,
this.allowedUserIds) for params.userId and if the user is not in the whitelist,
log a warning and return early (silently reject) instead of calling
this.messageHandler; otherwise continue as before. Ensure the check mirrors the
authorization logic used for app_mention/DM paths so button/modal interactions
cannot bypass gate authorization.

In `@packages/cli/src/commands/setup.ts`:
- Line 1033: The setup wizard currently lists "reactions:write" as a required
scope in the concatenated scope string ('   - app_mentions:read, chat:write,
reactions:write, channels:history\n' +); update that string/template so
"reactions:write" is marked as optional (e.g., move it into parentheses or
annotate with "[optional]" and remove it from any required-scopes enforcement
logic), and ensure any validation code that enforces required scopes (the
function or constant that builds/validates the scope list in setup.ts) does not
treat reactions:write as mandatory.

---

Outside diff comments:
In @.claude/skills/archon/guides/setup.md:
- Around line 161-173: The two subsection headings currently labeled "### 5c:
Verify Configuration" and "### 5d: Run Database Migrations (PostgreSQL only)"
are incorrectly numbered and should remain under Step 4; rename those headings
to "### 4c: Verify Configuration" and "### 4d: Run Database Migrations
(PostgreSQL only)" respectively so they follow "4b" and avoid conflict with the
actual "## Step 5" section; ensure any internal references to 5c/5d in nearby
text are updated to 4c/4d as well.

In `@packages/workflows/src/defaults/bundled-defaults.ts`:
- Line 40: The section header comment "// Default Workflows (13 total)" is out
of sync with BUNDLED_WORKFLOWS (which now contains 14 entries); update the
header to reflect the correct count (e.g., "// Default Workflows (14 total)"),
or replace the hardcoded count with a dynamic reference in documentation/tests
by using BUNDLED_WORKFLOWS.length where appropriate so the comment/test stays in
sync with the array.

---

Nitpick comments:
In @.archon/scripts/dispatch-review-app.js:
- Around line 34-38: The script currently prints both gh stdout and stderr to
stdout which corrupts the JSON summary; change the stderr handling so that when
stderr.trim() is truthy it is written to stderr (e.g., use
process.stderr.write(...) or console.error(...)) instead of console.log; keep
the stdout handling and the JSON.stringify line unchanged so the final JSON
summary remains the only thing emitted on stdout by dispatch-review-app.js
(refer to the stdout/stderr variables and the JSON.stringify({ dispatched: true,
workflow: workflowFile, ref }) line).

In @.cursor/mcp.json:
- Line 5: The args entry is using an unpinned image tag
"scoutapp/scout-mcp-local:latest" which makes runs non-reproducible; update the
args array to reference a specific version tag or immutable digest (for example
replace "scoutapp/scout-mcp-local:latest" with "scoutapp/scout-mcp-local:vX.Y.Z"
or "scoutapp/scout-mcp-local@sha256:...") so the invocation in the "args" array
is pinned and stable across runs.

In `@packages/workflows/src/deps.ts`:
- Around line 50-51: Extract the inline interactiveGate object shape into a
named exported interface (e.g., InteractiveGate) and replace the optional
property declaration interactiveGate?: { runId: string; nodeId: string } with
interactiveGate?: InteractiveGate; ensure the new interface is exported from the
same module so it becomes part of the public metadata contract and matches
MessageMetadata.interactiveGate semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 45b203d3-6e08-485b-914e-83de9d00837d

📥 Commits

Reviewing files that changed from the base of the PR and between 235a8ce and f11a43d.

⛔ Files ignored due to path filters (1)
  • bun.lock is excluded by !**/*.lock
📒 Files selected for processing (29)
  • .archon/commands/defaults/scout-consolidate-perf-plan.md
  • .archon/commands/defaults/scout-discover-routes.md
  • .archon/config.yaml
  • .archon/scripts/ci-wait.js
  • .archon/scripts/dispatch-review-app.js
  • .archon/scripts/fetch-review-app-url.js
  • .archon/workflows/defaults/archon-scout-perf-roadmap.yaml
  • .archon/workflows/defaults/archon-slack-feature-to-review-app.yaml
  • .claude/skills/archon/guides/setup.md
  • .claude/skills/archon/guides/slack.md
  • .cursor/hooks/state/continual-learning-index.json
  • .cursor/hooks/state/continual-learning.json
  • .cursor/mcp.json
  • AGENTS.md
  • docs/plans/2026-04-17-slack-archie-feature-to-review-app-plan.md
  • docs/plans/2026-04-20-slack-scoping-questions-form-plan.md
  • docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md
  • package.json
  • packages/adapters/src/chat/slack/adapter.test.ts
  • packages/adapters/src/chat/slack/adapter.ts
  • packages/cli/src/commands/setup.ts
  • packages/core/src/types/index.ts
  • packages/docs-web/src/content/docs/adapters/slack.md
  • packages/server/src/index.ts
  • packages/workflows/src/dag-executor.test.ts
  • packages/workflows/src/dag-executor.ts
  • packages/workflows/src/defaults/bundled-defaults.test.ts
  • packages/workflows/src/defaults/bundled-defaults.ts
  • packages/workflows/src/deps.ts

Comment thread .archon/config.yaml
Comment on lines +3 to +5
# Copy local .env into isolated worktrees so Scout MCP sees SCOUT_API_KEY when cwd is ~/.archon/workspaces/...
copyFiles:
- .env
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid copying the entire .env into every worktree.

This duplicates all local secrets for one Scout key and broadens exposure to generated workspaces. Prefer managed env injection, or copy a least-privilege Scout-only env file and point the MCP config at that file. Based on learnings, per-project env vars are stored in database, injected into bash/script nodes, Claude/Codex AI calls, and direct chat when codebase-scoped.

🛡️ Safer direction
-  # Copy local .env into isolated worktrees so Scout MCP sees SCOUT_API_KEY when cwd is ~/.archon/workspaces/...
+  # Copy only Scout MCP env needed by isolated worktrees; do not duplicate the full repo .env.
   copyFiles:
-    - .env
+    - .env.scout

Also update the Cursor MCP envFile to read the same least-privilege file if file-based injection is still required.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/config.yaml around lines 3 - 5, The config currently copies the full
local .env (copyFiles: - .env) into each worktree which duplicates secrets;
instead create a least-privilege file (e.g., scout.env) that contains only the
Scout-specific vars (SCOUT_API_KEY) and replace the .env reference in copyFiles
with that filename, or switch to managed env injection instead of file-copy;
also update the Cursor MCP's envFile setting to point to the same
least-privilege scout.env if file-based injection is still used so both MCPs
read the minimal Scout-only vars rather than the full .env.

Comment on lines +43 to +54
let timedOut = false;
const timer = setTimeout(() => {
timedOut = true;
console.error(`\nCI wait timed out after ${Math.round(timeoutMs / 1000)}s`);
child.kill('SIGTERM');
setTimeout(() => process.exit(3), 2000).unref();
}, timeoutMs);
timer.unref();

child.on('exit', (code, _signal) => {
clearTimeout(timer);
if (timedOut) return;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and examine the file
find . -name "ci-wait.js" -type f

Repository: coleam00/Archon

Length of output: 87


🏁 Script executed:

# Get the file content to understand context
cat -n .archon/scripts/ci-wait.js

Repository: coleam00/Archon

Length of output: 2383


Preserve the timeout exit code after killing gh.

When the timeout triggers and the child exits before the 2-second grace period, the exit handler returns early without calling process.exit(3). The grace timer on line 48 is .unref()'d and therefore does not keep the process alive—the process exits immediately with no referenced handles remaining, so the grace timer never fires. Instead of exiting with code 3 as documented, the process exits with the child's exit code or 0. Since exit code 3 is used by archon-slack-feature-to-review-app for deployment gating, fix by removing .unref() from the grace timer and calling process.exit(3) directly when timedOut in the exit handler:

Proposed fix
   let timedOut = false;
+  let timeoutExit: ReturnType<typeof setTimeout> | undefined;
   const timer = setTimeout(() => {
     timedOut = true;
     console.error(`\nCI wait timed out after ${Math.round(timeoutMs / 1000)}s`);
     child.kill('SIGTERM');
-    setTimeout(() => process.exit(3), 2000).unref();
+    timeoutExit = setTimeout(() => process.exit(3), 2000);
   }, timeoutMs);
   timer.unref();

   child.on('exit', (code, _signal) => {
     clearTimeout(timer);
-    if (timedOut) return;
+    if (timedOut) {
+      if (timeoutExit) clearTimeout(timeoutExit);
+      process.exit(3);
+    }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/scripts/ci-wait.js around lines 43 - 54, The timeout handler sets
timedOut and schedules a 2s grace timer with .unref(), but because unrefed
timers don't keep the process alive the grace timer may never run and the child
'exit' handler returns early, dropping the intended exit code; update the
timeout logic in the timeout callback to stop using .unref() on the grace
setTimeout and in the child.on('exit', ...) handler (referencing timedOut,
timer, and child) call process.exit(3) directly when timedOut is true before
returning so the process reliably exits with code 3 after a timeout.

Comment on lines +86 to +95
while (Date.now() < deadline) {
try {
const match = await pollOnce(pr, regex);
if (match) {
console.log(match);
return;
}
} catch (err) {
console.error(`Poll error (will retry): ${err.message}`);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Find the file and check its existence
find . -name "fetch-review-app-url.js" -type f

Repository: coleam00/Archon

Length of output: 100


🏁 Script executed:

# Read the file to verify lines 86-95 and understand the context
cat -n .archon/scripts/fetch-review-app-url.js | head -120

Repository: coleam00/Archon

Length of output: 3729


🏁 Script executed:

# Search for pollOnce function definition and error types
rg -A 10 "function pollOnce|const pollOnce|pollOnce\s*=" .archon/scripts/fetch-review-app-url.js

Repository: coleam00/Archon

Length of output: 265


Fail fast for permanent polling failures.

The loop retries every error from pollOnce, so missing gh, JSON parsing failures, and other permanent setup issues wait the full polling window before exiting as "URL not found" (3) instead of "setup failure" (2) per the documented exit codes. Detect and exit immediately with code 2 for non-retryable errors like missing gh binary (ENOENT) and invalid comments JSON.

🐛 Proposed direction
     } catch (err) {
-      console.error(`Poll error (will retry): ${err.message}`);
+      const error = err;
+      const message = error instanceof Error ? error.message : String(error);
+      const code = typeof error === 'object' && error !== null && 'code' in error
+        ? String((error as { code?: unknown }).code)
+        : undefined;
+
+      if (code === 'ENOENT') {
+        console.error(`Failed to spawn gh: ${message}`);
+        process.exit(2);
+      }
+
+      console.error(`Poll error (will retry): ${message}`);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
while (Date.now() < deadline) {
try {
const match = await pollOnce(pr, regex);
if (match) {
console.log(match);
return;
}
} catch (err) {
console.error(`Poll error (will retry): ${err.message}`);
}
while (Date.now() < deadline) {
try {
const match = await pollOnce(pr, regex);
if (match) {
console.log(match);
return;
}
} catch (err) {
const error = err;
const message = error instanceof Error ? error.message : String(error);
const code = typeof error === 'object' && error !== null && 'code' in error
? String((error as { code?: unknown }).code)
: undefined;
if (code === 'ENOENT') {
console.error(`Failed to spawn gh: ${message}`);
process.exit(2);
}
console.error(`Poll error (will retry): ${message}`);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/scripts/fetch-review-app-url.js around lines 86 - 95, The current
polling loop around pollOnce swallows all errors and retries, which delays
exiting for permanent failures; update the catch block in the while loop that
wraps pollOnce (in .archon/scripts/fetch-review-app-url.js) to detect
non-retryable errors and exit immediately with code 2: specifically check for
err.code === 'ENOENT' (missing gh binary) and JSON parse failures (err
instanceof SyntaxError or err.name === 'SyntaxError' coming from JSON.parse) and
call process.exit(2) with a clear error log; for other transient errors keep the
existing console.error and continue retrying.

Comment on lines +27 to +32
- id: discover-routes
command: scout-discover-routes
context: fresh
mcp: .archon/mcp/scout-apm.json

- id: profile-00
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Add a DAG guard for empty Scout discovery results.

If discovery writes routes.json as [], all profile nodes can skip and consolidate-plan can still run. Add a bash validation node after discover-routes and make profilers depend on it, so the workflow fails before generating an empty optimization plan.

🐛 Proposed direction
   - id: discover-routes
     command: scout-discover-routes
     context: fresh
     mcp: .archon/mcp/scout-apm.json
+
+  - id: ensure-routes-discovered
+    depends_on: [discover-routes]
+    bash: |
+      set -euo pipefail
+      count=$(jq 'length' "$ARTIFACTS_DIR/routes.json")
+      if [ "$count" = "0" ]; then
+        echo "ERROR: Scout returned no endpoints; stopping before profiling." >&2
+        exit 1
+      fi
+      echo "Scout discovery returned $count route(s)."

Then change each profile-* node to depend on ensure-routes-discovered instead of discover-routes.

Also applies to: 276-279

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/workflows/defaults/archon-scout-perf-roadmap.yaml around lines 27 -
32, Add a new bash validation node (id: ensure-routes-discovered) immediately
after the discover-routes node that checks the produced routes.json is not an
empty array (e.g., fail if file missing or content == "[]"), and make all
profile nodes (profile-00, profile-01, etc.) depend on ensure-routes-discovered
instead of discover-routes so the DAG fails early when discovery yields no
routes; keep consolidate-plan dependency unchanged so it can still run when
appropriate.

Comment on lines +373 to +381
- id: fetch-review-url
depends_on: [deploy-review-app]
timeout: 1000000
bash: |
set -e
bun .archon/scripts/fetch-review-app-url.js \
"$extract-pr-number.output" \
'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \
900000 20000
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

find . -name "fetch-review-app-url.js" -type f

Repository: coleam00/Archon

Length of output: 100


🏁 Script executed:

wc -l ./.archon/scripts/fetch-review-app-url.js

Repository: coleam00/Archon

Length of output: 104


🏁 Script executed:

cat -n ./.archon/scripts/fetch-review-app-url.js

Repository: coleam00/Archon

Length of output: 3729


Use JavaScript regex syntax for the review-app URL pattern.

fetch-review-app-url.js passes this argument to new RegExp(...) at line 62, but [:space:] is POSIX syntax, not JavaScript regex syntax. In JavaScript, [^[:space:]] matches any character that is NOT one of the literal characters :, s, p, a, c, e. Use \s so the polling can reliably extract the review-app URL and stop at whitespace as intended.

🐛 Proposed fix
       bun .archon/scripts/fetch-review-app-url.js \
         "$extract-pr-number.output" \
-        'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \
+        'https://[^\s)]+\.review\.instrumentl\.com[^\s)]*' \
         900000 20000
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- id: fetch-review-url
depends_on: [deploy-review-app]
timeout: 1000000
bash: |
set -e
bun .archon/scripts/fetch-review-app-url.js \
"$extract-pr-number.output" \
'https://[^[:space:])]+\.review\.instrumentl\.com[^[:space:])]*' \
900000 20000
- id: fetch-review-url
depends_on: [deploy-review-app]
timeout: 1000000
bash: |
set -e
bun .archon/scripts/fetch-review-app-url.js \
"$extract-pr-number.output" \
'https://[^\s)]+\.review\.instrumentl\.com[^\s)]*' \
900000 20000
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml around
lines 373 - 381, The bash step passes a POSIX-style character class to
fetch-review-app-url.js which is later used in new RegExp(...) (see new RegExp
usage around line 62 in fetch-review-app-url.js); replace the POSIX `[:space:]`
usage with JavaScript whitespace `\s` in the pattern argument and ensure the
backslash is escaped correctly for the YAML/bash context (so the JS gets `\s` at
runtime and dots are escaped for the domain). Update the `fetch-review-url`
step's pattern string accordingly so the script receives a valid JS regex that
stops at whitespace.

Comment on lines +44 to +45
One new bundled workflow in `packages/workflows/src/defaults/workflows/` that
composes existing commands (spec questions from `archon-interactive-prd`, plan +
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Update stale implementation paths and script extensions.

The workflow now lives under .archon/workflows/defaults/, and the helper scripts in this PR are .js, not .ts. These stale references will send readers to missing files.

📝 Proposed fix
-One new bundled workflow in `packages/workflows/src/defaults/workflows/` that
+One new bundled workflow in `.archon/workflows/defaults/` that
@@
-File: `packages/workflows/src/defaults/workflows/archon-slack-feature-to-review-app.yaml`.
+File: `.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml`.
@@
-- `packages/workflows/src/defaults/workflows/archon-slack-feature-to-review-app.yaml`
-- `.archon/scripts/ci-wait.ts`
-- `.archon/scripts/fetch-review-app-url.ts`
-- `.archon/scripts/dispatch-review-app.ts`
+- `.archon/workflows/defaults/archon-slack-feature-to-review-app.yaml`
+- `.archon/scripts/ci-wait.js`
+- `.archon/scripts/fetch-review-app-url.js`
+- `.archon/scripts/dispatch-review-app.js`

Also applies to: 88-89, 286-291

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md` around
lines 44 - 45, References to the workflow location and helper script extensions
are stale: update any occurrences of the old workflow path string
"packages/workflows/src/defaults/workflows/" to the new location
".archon/workflows/defaults/" and change helper script filenames/extensions
referenced as ".ts" to ".js" (e.g., helper script imports or examples), then
scan and fix the other occurrences called out (the additional instances noted in
the comment) so all path and extension references are consistent.


Example sequence:

```
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add a language to the fenced block.

Markdown lint flags this fence; use text for the plain progress-message example.

📝 Proposed fix
-```
+```text
 🧠 Spec approved. Creating implementation plan...
 🏗️  Plan ready. Spinning up worktree <branch> and implementing...
 ✅ Implementation passed local validation. Opening PR...
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)

[warning] 181-181: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@docs/specs/2026-04-17-slack-archie-feature-to-review-app-design.md` at line
181, The fenced code block at the mentioned location is missing a language tag
causing markdown lint errors; update the triple-backtick fence to include the
language identifier "text" (i.e., use ```text) for the plain progress-message
example so the block is explicitly marked as plain text.

Comment on lines +492 to +494
getLog().info(
{ runId: ids?.runId, nodeId: ids?.nodeId, userId: ctx.userId },
'slack.gate_approve_clicked'
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Mask Slack user IDs in gate logs.

These new logs include raw userId; keep the existing masking pattern used by unauthorized message logs.

🛡️ Proposed fix
+    const maskedUserId = `${ctx.userId.slice(0, 4)}***`;
     getLog().info(
-      { runId: ids?.runId, nodeId: ids?.nodeId, userId: ctx.userId },
+      { runId: ids?.runId, nodeId: ids?.nodeId, maskedUserId },
       'slack.gate_approve_clicked'
     );
@@
+    const maskedUserId = `${ctx.userId.slice(0, 4)}***`;
     getLog().info(
-      { runId: ids?.runId, nodeId: ids?.nodeId, userId: ctx.userId },
+      { runId: ids?.runId, nodeId: ids?.nodeId, maskedUserId },
       'slack.gate_request_changes_clicked'
     );

As per coding guidelines, never log user message content or PII in logs.

Also applies to: 550-552

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/adapters/src/chat/slack/adapter.ts` around lines 492 - 494, The logs
currently pass raw ctx.userId into getLog().info for the
'slack.gate_approve_clicked' event (and the similar getLog().info call around
lines 550-552); replace the raw userId with the existing Slack user-id masking
utility used by the unauthorized-message logs (i.e., call the same masking
function used elsewhere—e.g., maskUserId(ctx.userId) or the project’s
PII/masking helper) so the getLog().info payload contains the masked user id
instead of ctx.userId for both 'slack.gate_approve_clicked' and the similar gate
log.

Comment on lines +668 to +688
private async dispatchSyntheticMessage(params: {
channel: string;
threadTs: string;
userId: string;
text: string;
}): Promise<void> {
if (!this.messageHandler) {
getLog().warn('slack.gate_synthetic_no_handler');
return;
}
const event: SlackMessageEvent = {
text: params.text,
user: params.userId,
channel: params.channel,
// `ts` of a synthetic reply: reuse thread_ts; the orchestrator does not
// persist this field, and no Slack API call depends on it.
ts: params.threadTs,
thread_ts: params.threadTs,
};
try {
await this.messageHandler(event);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Apply the Slack whitelist before synthetic gate dispatch.

Button/modal interactions bypass the app_mention/DM authorization checks and call messageHandler directly. Check allowedUserIds here so unauthorized Slack users cannot approve or modify workflow gates.

🛡️ Proposed fix
   private async dispatchSyntheticMessage(params: {
     channel: string;
     threadTs: string;
     userId: string;
     text: string;
   }): Promise<void> {
     if (!this.messageHandler) {
       getLog().warn('slack.gate_synthetic_no_handler');
       return;
     }
+    if (!isSlackUserAuthorized(params.userId, this.allowedUserIds)) {
+      const maskedId = params.userId ? `${params.userId.slice(0, 4)}***` : 'unknown';
+      getLog().info({ maskedUserId: maskedId }, 'slack.unauthorized_gate_interaction');
+      return;
+    }
     const event: SlackMessageEvent = {
       text: params.text,
       user: params.userId,

As per coding guidelines, packages/adapters/src/**/*.ts adapters must check authorization before calling the message handler and silently reject unauthorized users.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private async dispatchSyntheticMessage(params: {
channel: string;
threadTs: string;
userId: string;
text: string;
}): Promise<void> {
if (!this.messageHandler) {
getLog().warn('slack.gate_synthetic_no_handler');
return;
}
const event: SlackMessageEvent = {
text: params.text,
user: params.userId,
channel: params.channel,
// `ts` of a synthetic reply: reuse thread_ts; the orchestrator does not
// persist this field, and no Slack API call depends on it.
ts: params.threadTs,
thread_ts: params.threadTs,
};
try {
await this.messageHandler(event);
private async dispatchSyntheticMessage(params: {
channel: string;
threadTs: string;
userId: string;
text: string;
}): Promise<void> {
if (!this.messageHandler) {
getLog().warn('slack.gate_synthetic_no_handler');
return;
}
if (!isSlackUserAuthorized(params.userId, this.allowedUserIds)) {
const maskedId = params.userId ? `${params.userId.slice(0, 4)}***` : 'unknown';
getLog().info({ maskedUserId: maskedId }, 'slack.unauthorized_gate_interaction');
return;
}
const event: SlackMessageEvent = {
text: params.text,
user: params.userId,
channel: params.channel,
// `ts` of a synthetic reply: reuse thread_ts; the orchestrator does not
// persist this field, and no Slack API call depends on it.
ts: params.threadTs,
thread_ts: params.threadTs,
};
try {
await this.messageHandler(event);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/adapters/src/chat/slack/adapter.ts` around lines 668 - 688, In
dispatchSyntheticMessage, enforce the Slack whitelist before invoking
messageHandler: check the adapter's allowedUserIds (e.g., this.allowedUserIds)
for params.userId and if the user is not in the whitelist, log a warning and
return early (silently reject) instead of calling this.messageHandler; otherwise
continue as before. Ensure the check mirrors the authorization logic used for
app_mention/DM paths so button/modal interactions cannot bypass gate
authorization.

' - Generate an App-Level Token (xapp-...)\n' +
'3. Add Bot Token Scopes (OAuth & Permissions):\n' +
' - app_mentions:read, chat:write, channels:history\n' +
' - app_mentions:read, chat:write, reactions:write, channels:history\n' +
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Mark reactions:write as optional in the setup wizard.

The adapter works without this scope; requiring it in setup grants extra write permission for a receipt-only enhancement.

🛡️ Proposed wording
-      '   - app_mentions:read, chat:write, reactions:write, channels:history\n' +
+      '   - app_mentions:read, chat:write, channels:history\n' +
+      '   - Optional: reactions:write (for :eyes: receipt reactions)\n' +
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
' - app_mentions:read, chat:write, reactions:write, channels:history\n' +
' - app_mentions:read, chat:write, channels:history\n' +
' - Optional: reactions:write (for :eyes: receipt reactions)\n' +
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/commands/setup.ts` at line 1033, The setup wizard currently
lists "reactions:write" as a required scope in the concatenated scope string (' 
- app_mentions:read, chat:write, reactions:write, channels:history\n' +); update
that string/template so "reactions:write" is marked as optional (e.g., move it
into parentheses or annotate with "[optional]" and remove it from any
required-scopes enforcement logic), and ensure any validation code that enforces
required scopes (the function or constant that builds/validates the scope list
in setup.ts) does not treat reactions:write as mandatory.

@Wirasm Wirasm closed this Apr 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants